🧱 refactor: storage_session_id rename + kind discriminator (3.1.80-dev.0)#148
Merged
danny-avila merged 2 commits intodevfrom May 7, 2026
Merged
🧱 refactor: storage_session_id rename + kind discriminator (3.1.80-dev.0)#148danny-avila merged 2 commits intodevfrom
danny-avila merged 2 commits intodevfrom
Conversation
a16adfc to
1f1faed
Compare
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
fcbd93d to
817d5d3
Compare
8 tasks
…v.0) Combines Phase B (per-file session_id rename) and Phase C (kind discriminator for designed cross-user-within-tenant caching) in one cutover. Pre-release simplifies coordination — no wire-compat aliases. Type changes: - `CodeEnvFile`: per-file `session_id` → `storage_session_id`. Adds `kind: 'skill' | 'agent' | 'user' | 'system'` (required) and `version?: number` (used for `kind: 'skill'`). New `CodeEnvKind` type alias exported. - `FileRef`: same `storage_session_id` rename. `kind?` and `version?` optional (legacy passthroughs may not carry them). - Top-level `session_id` on `CodeSessionContext` / `ExecuteResult` / artifact / response shapes is unchanged — that's the canonical execution-session name and a future engine that uses one session concept lands on it directly. ToolNode propagates `kind` and `version?` through: - `runTool` injection (`_injected_files`) - `getCodeSessionContext` (event-driven path) - `updateCodeSession` and `storeCodeSessionFromResults` (already pass-through via spread; new optional fields flow naturally) Defaults: - ToolNode and the `/files` fallback tag missing-kind files as `kind: 'user'`. Most ad-hoc files are user-private; shared resources (`skill`, `agent`) populate their kind upstream in LC. Tests: - 787 / 787 tools tests pass; tsc clean. - 20 / 20 in `ToolNode.session.test.ts` (per-file storage and kind pinned at every boundary). Coordination: - Companion: LibreChat #12960 (sets `kind` at every write site, threads `skill.version` through skill primes), codeapi #1455 (rewrites `resolveSessionKey` as a kind-switch with `authContext.tenantId` prefix). All three deploy in lockstep — pre-release, no aliases retained.
Tightens the per-file ref contract codeapi expects on `_injected_files`:
- `CodeEnvFile` is now a discriminated union on `kind`. `version` is
statically required for `kind: 'skill'` and statically forbidden for
`agent` / `user`. The constraint holds at compile time on every
consumer, not just at codeapi's runtime validator boundary.
- `CODE_ENV_KINDS` const-tuple keeps the runtime list and the TS union
locked together — adding a new kind to the tuple updates both at
once, so a partial update won't silently widen one side.
- JSDoc on `id` calls out the per-kind asymmetry: skill UUID for
`'skill'`, agent id for `'agent'`, informational-only for `'user'`
(codeapi resolves user identity from the auth context). Future
readers seeing `RequestFile { id, kind: 'user' }` shouldn't assume
`id` routes anywhere.
ToolNode collapses two near-identical CodeEnvFile constructors
(the runTool-injection path and the event-driven `getCodeSessionContext`
path) onto a single `toInjectedFileRef` helper. The helper does the
discriminated-union narrowing in one place instead of duplicating the
"if kind===skill set version" logic at every call site, where the
previous shape — `const ref = { ... }; if (file.version != null) ref.version = file.version;` —
no longer typechecks against the union variants. Skill refs missing
`version` fall back to `'user'` so an upstream contract bug surfaces
as a degraded sessionKey rather than a runtime crash; primeSkillFiles
is the only writer and always sets `version`.
Companion: LibreChat #12960 (mirrors the discriminated union on
`CodeEnvRef`, drops `entity_id` everywhere). codeapi #1455 already
validates the same shape on the wire.
e8d05eb to
7af210b
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Type-system shape for the lockstep cutover of LibreChat ↔ codeapi sandbox file identity. Two distinct concepts had been sharing the field name
session_id; this PR also adds thekinddiscriminator so codeapi's sessionKey derives from explicit resource type rather than emergent legacy behavior.Companions: codeapi #1455, LibreChat #12960. Pre-release lockstep, no legacy aliases.
Bumps to 3.1.80-dev.0.
Final shape
What's in this PR
session_id→storage_session_idrename. Top-levelsession_id(transient sandbox-run id onCodeSessionContext,ExecuteResult,ProgrammaticExecutionResponse, etc.) keeps its name — only the misnamed side moves. Hard rename, no aliases.kinddiscriminator added toCodeEnvFile(required) andFileRef(optional, since/filesfallback may not carry it). Drives codeapi's per-resource sessionKey:<tenant>:<kind>:<id>[:v:<version>]for shared kinds,<tenant>:user:<userId>for user-private.CodeEnvFile:version: numberis statically required forkind: 'skill'and statically forbidden for'agent'/'user'. Constraint holds at compile time on every consumer, not just at codeapi's runtime validator.CODE_ENV_KINDSconst-tuple — adding a new kind to the tuple updates both the runtime list and the TS union at once, so a partial update can't silently widen one side.entity_idfromCodeEnvFile,FileRef, and ToolNode propagation. Drops'system'from the kind union (no emitter ever existed).toInjectedFileRefhelper inToolNode: collapses the two near-identicalCodeEnvFileconstructors (therunToolinjection path and the event-drivengetCodeSessionContextpath) onto one call site that does the discriminated-union narrowing in one place. Defaults missing-kind to'user'; a skill ref missingversionfalls back to'user'so an upstream contract bug surfaces as a degraded sessionKey rather than a runtime crash.idcalls out the per-kind asymmetry; onversiondocuments the'skill'-only requirement.Sites updated
src/types/tools.ts—CodeEnvKind,CODE_ENV_KINDS, discriminatedCodeEnvFile, JSDoc onFileRef.src/tools/ToolNode.ts—toInjectedFileRefhelper atrunToolinjection (_injected_files) andgetCodeSessionContext.updateCodeSessionandstoreCodeSessionFromResultspass new fields through via spread.CodeExecutor,BashExecutor,ProgrammaticToolCalling,BashProgrammaticToolCalling,LocalExecutionTools,LocalProgrammaticToolCalling— per-file refs usestorage_session_id; top-levelsession_idextraction unchanged.ToolNode.session.test.tsrepurposedentity_id-forwarding cases askind+versionforwarding cases (the actual Phase C contract).Test plan
npx tsc --noEmitcleannpx jest src/tools src/types— 787 / 787 pass (incl. 18 session-management tests with new per-file shape)@librechat/agentsdep to3.1.80-dev.0after this lands and run a multi-turn execute against codeapi to confirm.Why no legacy aliases
The deployment matrix is controlled — codeapi service + LC's pinned
@librechat/agentsversion + LC itself all ship together. A synchronized cutover is fine; alias plumbing for a deploy ordering that doesn't exist is just code to maintain and clean up.Why keep top-level
session_idRenaming both sides was symmetric but unnecessary. The bug was that per-file
session_idwas the wrong name (it really meant "storage"). Top-levelsession_idis the right name (it really means "session"). Disambiguating only the misnamed side gives ~half the churn at every call site, and a future engine that uses one session concept lands onsession_iddirectly with no rename to undo.